Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.71%. Comparing base (5df279d) to head (7f99790).
⚠️ Report is 12 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3435      +/-   ##
==========================================
+ Coverage   70.54%   70.71%   +0.17%     
==========================================
  Files         232      232              
  Lines       20022    20037      +15     
==========================================
+ Hits        14125    14170      +45     
+ Misses       5897     5867      -30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Was this a bug or a feature, isn't it reasonable to assume one feeds in strings for these?

This was actually brought to my attention because of a lightkurve issue. Previously, you could input an integer directly and have it be resolved if it was within a certain range of TIC/KID IDs.

I believe that should be dealt with inside light curve, for here it feels wrong. I mean why just TIC/KID id then, why not panstarrs object IDs or any other surveys that have a numerical ID?

cc @keflavich if he wants to chime in

My take is that this is a lightkurv issue, specifically:

--> 182 object_names = [objectname] if isinstance(objectname, str) else list(objectname)

is type checking. Probably this is a case where a property check is more appropriate, e.g.

try:
    object_names = list(objectname)
except TypeError:
    object_names = [objectname]

I agree with @bsipocz that this is not a good approach to include in astroquery: object IDs are database-specific, and it looks like the implementation proposed here already creates a dangerous ambiguity, i.e., is it a TIC ID or a K2 ID?

We can be overridden here if MAST has a clear, specific need to support integer ID based queries, but I'd recommend that it not be built in to resolve_object and certainly should not be applied generally to other modules.

That particular line (182) is actually in Astroquery. I agree that using a property check with a try/except is safer than strict type checking here; I’ll update that accordingly.

As for the integer ID ambiguity, that’s a good point. The intention was to allow queries by TIC/Kepler ID specifically, since our resolver service knows to treat them as such. I'll note that passing in the same integer as a string does work with the current implementation, but I agree that it's better practice to include an identifier (like "TIC" or "KIC"). I'll advise the folks at lightkurve to use that approach.

Rebased for conflict resolution, I'm merging once CI is all green again.